flow: log_cmd the unlogged synth and load steps for timing#4292
flow: log_cmd the unlogged synth and load steps for timing#4292oharboe wants to merge 2 commits into
Conversation
The synth_odb step runs load_design and the PRE/POST SYNTH step-tcl hooks, and load.tcl runs read_verilog, without wrapping them in log_cmd. Their wall time is therefore not attributed in the per-command timing log, leaving a gap when accounting for where the synth step spends its time. link_design, read_sdc, eliminate_dead_logic, write_db and write_sdc already log themselves; wrap the remaining ops to match. No behaviour change. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request wraps several commands in log_cmd within flow/scripts/load.tcl and flow/scripts/synth_odb.tcl to improve logging. The reviewer recommends against wrapping load_design and source_step_tcl in log_cmd because doing so introduces redundant nested logging and noise. Instead, the reviewer suggests wrapping the remaining unlogged internal commands, such as read_lef inside load_design, directly.
| log_cmd load_design 1_2_yosys.v 1_2_yosys.sdc | ||
| log_cmd source_step_tcl PRE SYNTH |
There was a problem hiding this comment.
Wrapping load_design and source_step_tcl in log_cmd introduces redundant nested logging and unnecessary noise:
source_step_tcl: This procedure internally callssource_env_var_if_exists, which already wraps thesourcecommand inlog_cmdif the hook is defined. Wrapping it here causes redundant nested logs when hooks are defined, and prints empty/noisy log entries when they are not.load_design: Almost all internal commands ofload_design(likeread_verilog,link_design,read_sdc, etc.) are already wrapped inlog_cmd. Wrapping the entire procedure results in nested logs.
Instead of wrapping load_design here, we should wrap the remaining unlogged commands (like read_lef) inside load_design itself in flow/scripts/load.tcl.
load_design 1_2_yosys.v 1_2_yosys.sdc
source_step_tcl PRE SYNTH
| report_design_area_metrics | ||
|
|
||
| source_step_tcl POST SYNTH | ||
| log_cmd source_step_tcl POST SYNTH |
There was a problem hiding this comment.
As mentioned, source_step_tcl already logs the actual source command internally if the hook is defined. Wrapping it in log_cmd here is redundant and adds noise to the logs when the hook is not defined.
source_step_tcl POST SYNTH
| } | ||
| } | ||
| read_verilog $::env(RESULTS_DIR)/$design_file | ||
| log_cmd read_verilog $::env(RESULTS_DIR)/$design_file |
There was a problem hiding this comment.
While wrapping read_verilog in log_cmd is a great improvement, the read_lef commands in load_design (lines 19, 20, and 23) are still not logged. Sourcing LEF files can take a significant amount of time, so wrapping them in log_cmd inside load_design would provide complete visibility without needing to wrap the entire load_design procedure in synth_odb.tcl (which causes nested logging).
Per review, wrapping load_design and source_step_tcl in log_cmd double-logs: source_step_tcl already log_cmd's its source via source_env_var_if_exists, and load_design's other internals (read_verilog, link_design, read_sdc) are already wrapped. The only unlogged ops are the read_lef calls in load_design. Wrap those directly and revert the synth_odb.tcl wraps; the net change is read_lef/read_verilog logging only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
Reworked per review: |
|
Not pursuing this one — closing. |
1 participant
The
synth_odbstep runsload_designand thePRE/POST SYNTHstep-tcl hooks, andload.tclrunsread_verilog, without wrapping them inlog_cmd. Their wall time is therefore not attributed in the per-command timing log, leaving a gap when accounting for where the synth step spends its time.link_design,read_sdc,eliminate_dead_logic,write_dbandwrite_sdcalready log themselves; this wraps the remaining ops to match. No behavior change.